Preserve RabbitMQ topic names in queue stats#1420
Conversation
|
@abishekgiri is attempting to deploy a commit to the motia Team on Vercel. A member of the Team first needs to authorize it. |
|
Updated RabbitMQ topic reporting so subscription keys are parsed from the right instead of the left. That keeps topic stats and list-topics output correct when the topic name itself contains colons, and it adds a regression test for that shape. |
📝 WalkthroughWalkthroughA helper function Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
engine/src/workers/queue/adapters/rabbitmq/adapter.rs (1)
1238-1244: Add a regression test for colon-containing subscriber IDs.Current test covers colons in topic only. Add one case where the suffix id includes
:to lock this edge case.🧪 Suggested test case
#[test] fn subscription_topic_preserves_topics_with_colons() { assert_eq!( subscription_topic("orders:v1:123e4567-e89b-12d3-a456-426614174000"), Some("orders:v1") ); } + +#[test] +fn subscription_topic_handles_colons_in_subscriber_id() { + assert_eq!( + subscription_topic("orders:v1:fn::validate"), + Some("orders:v1") + ); +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/src/workers/queue/adapters/rabbitmq/adapter.rs` around lines 1238 - 1244, Add a regression test that ensures subscription_topic correctly strips the suffix even when the subscriber id contains a colon — e.g., add a new test (e.g., subscription_topic_handles_colon_in_suffix) that calls subscription_topic with a value where the part after the second colon itself contains a colon (for example "orders:v1:123e4567-e89b:12d3-a456-426614174000") and asserts it still returns Some("orders:v1"); reference the existing subscription_topic function and the current subscription_topic_preserves_topics_with_colons test to place and model the new test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@engine/src/workers/queue/adapters/rabbitmq/adapter.rs`:
- Around line 1238-1244: Add a regression test that ensures subscription_topic
correctly strips the suffix even when the subscriber id contains a colon — e.g.,
add a new test (e.g., subscription_topic_handles_colon_in_suffix) that calls
subscription_topic with a value where the part after the second colon itself
contains a colon (for example "orders:v1:123e4567-e89b:12d3-a456-426614174000")
and asserts it still returns Some("orders:v1"); reference the existing
subscription_topic function and the current
subscription_topic_preserves_topics_with_colons test to place and model the new
test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 3847ed13-2fe5-4c96-8fee-fb9dd9e9bd81
📒 Files selected for processing (1)
engine/src/workers/queue/adapters/rabbitmq/adapter.rs
|
Thanks @abishekgiri The right-to-left split approach makes sense for topic names with colons. One question though if subscriber IDs ever contain colons will this splitting always work correctly? (ex. @sergiofilhowz Do you have thoughts here? I'm not very familiar with RabbitMQ and our colons are more of a suggested design choice for paths than an enforced standard. |
Summary
orders:v1Testing
cargo test -p iii subscription_topic_preserves_topics_with_colons -- --nocaptureSummary by CodeRabbit
Refactor
Tests